Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support start/stop fragment marker #813

Merged
merged 32 commits into from
Dec 5, 2024

Conversation

wyfo
Copy link
Contributor

@wyfo wyfo commented Nov 25, 2024

Copy link

PR missing one of the required labels: {'bug', 'new feature', 'dependencies', 'documentation', 'breaking-change', 'internal', 'enhancement'}

Copy link

github-actions bot commented Dec 4, 2024

PR missing one of the required labels: {'dependencies', 'internal', 'documentation', 'enhancement', 'breaking-change', 'new feature', 'bug'}

@wyfo wyfo added the new feature Something new is needed label Dec 4, 2024
@wyfo wyfo force-pushed the feat/fragment_marker branch from 63fbb17 to 1f4cf1d Compare December 4, 2024 07:11
src/protocol/codec/transport.c Outdated Show resolved Hide resolved
src/protocol/codec/transport.c Outdated Show resolved Hide resolved
src/protocol/codec/transport.c Outdated Show resolved Hide resolved
src/protocol/codec/transport.c Outdated Show resolved Hide resolved
src/protocol/definitions/transport.c Outdated Show resolved Hide resolved
src/transport/unicast/rx.c Outdated Show resolved Hide resolved
src/transport/unicast/rx.c Outdated Show resolved Hide resolved
src/protocol/codec/transport.c Outdated Show resolved Hide resolved
src/protocol/definitions/transport.c Outdated Show resolved Hide resolved
src/protocol/definitions/transport.c Outdated Show resolved Hide resolved
wyfo and others added 16 commits December 4, 2024 10:17
Co-authored-by: Alexander Bushnev <[email protected]>
Co-authored-by: Alexander Bushnev <[email protected]>
Co-authored-by: Alexander Bushnev <[email protected]>
Co-authored-by: Alexander Bushnev <[email protected]>
Co-authored-by: Alexander Bushnev <[email protected]>
Co-authored-by: Alexander Bushnev <[email protected]>
Co-authored-by: Alexander Bushnev <[email protected]>
src/protocol/definitions/transport.c Outdated Show resolved Hide resolved
if (_Z_HAS_FLAG(header, _Z_FLAG_T_Z)) {
ret = _Z_ERR_MESSAGE_SERIALIZATION_FAILED;
if (msg->first) {
if (_Z_HAS_FLAG(header, _Z_FLAG_T_Z) == true) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a nit and I see there are some that precedes that PR, but all conditions should be booleans so the == true / false would be redundant and can be removed.

Copy link
Contributor Author

@wyfo wyfo Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #813 (comment)
I asked before writing the code, but didn't understand Sasha's answer ^_^' (I would never have the idea to write == true if I had not seen it everywhere in the code)

Copy link
Member

@Mallets Mallets Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, MISRA-C rules about using non-boolean values as boolean expression:
e. g.

int i = 1;
if (i) {}

but this kind of expression is absolutely legal:

bool b = true;
if (b) {}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, yes but I also believe it depends on the static analyser... I'm not saying we should use == true, I was just providing a bit more context :)

@Mallets Mallets enabled auto-merge December 5, 2024 09:09
@Mallets Mallets disabled auto-merge December 5, 2024 09:09
@Mallets
Copy link
Member

Mallets commented Dec 5, 2024

All CI is green but CI status checks which is stuck. I'm going to manual merge the PR.

@Mallets Mallets merged commit 54191c8 into eclipse-zenoh:main Dec 5, 2024
60 checks passed
@sashacmc
Copy link
Member

sashacmc commented Dec 5, 2024

All CI is green but CI status checks which is stuck. I'm going to manual merge the PR.

This is a known issue, @diogomatsubara is working on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature Something new is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants